Skip to content

Fix escapeString to properly escape HTML-unsafe characters - #1231

Open
pavankumar-vh wants to merge 1 commit into
CodebuffAI:mainfrom
pavankumar-vh:fix/escape-string-angular-brackets
Open

Fix escapeString to properly escape HTML-unsafe characters#1231
pavankumar-vh wants to merge 1 commit into
CodebuffAI:mainfrom
pavankumar-vh:fix/escape-string-angular-brackets

Conversation

@pavankumar-vh

@pavankumar-vh pavankumar-vh commented Sep 3, 2026

Copy link
Copy Markdown

Overview

Fix a potential XSS vulnerability in the escapeString function in common/src/util/string.ts.

Bug Description

The escapeString function used JSON.stringify to escape a string, which handles quotes and backslashes but doesn't escape characters that are unsafe in HTML contexts: <, >, &, and '.

This is a security concern if the escaped string is used in HTML or XML contexts, as it could allow XSS attacks or HTML injection.

Fix

Added explicit escaping for these characters to prevent potential security issues:

  • < → \\u003c
  • > → \\u003e
  • & → \\u0026
  • ' → \\u0027

Testing

No existing tests for this function, but the fix prevents potential XSS vulnerabilities.

Files Changed

  • common/src/util/string.ts - Added HTML-unsafe character escaping

Scope

This change only touches common/ which is an approved contribution area per the Contributing Guide.

@pavankumar-vh
Fix escapeString to properly escape HTML-unsafe characters
cfe7c6e 
The escapeString function used JSON.stringify to escape a string, which handles
quotes and backslashes but doesn't escape characters that are unsafe in HTML
contexts: <, >, &, and '.

This is a security concern if the escaped string is used in HTML or XML contexts,
as it could allow XSS attacks or HTML injection.

Added explicit escaping for these characters to prevent potential security issues.
@codebuff-team

codebuff-team commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Thanks for looking into escapeString. The instinct to worry about unescaped <, >, &, ' is reasonable, but this function's name and current behavior (JSON.stringify(str).slice(1,-1)) suggest it's meant for generic string escaping — likely used to embed a JS/JSON string literal inside generated code or templates, not for HTML output. Without seeing the call sites, I can't tell whether this change is safe: if any caller expects the literal </>/& characters to survive (e.g. building a string that's later JSON-parsed, or inserted into a non-HTML context), this silently changes their output and could introduce a regression rather than fix one.

A few concrete asks before this is portable:

  • Grep the codebase for escapeString usages and confirm none depend on the un-escaped characters passing through.
  • If the goal is genuinely HTML-safety, consider a separate function (escapeHtml or similar) rather than overloading this one, so existing callers aren't affected.
  • Add a unit test in common/src/util/string.test.ts (or wherever sibling tests live) covering the new escaping behavior and a regression test for the old behavior.
  • The PR body claims an XSS vulnerability but doesn't point to where this string ends up rendered as HTML — that context would make the case much stronger.

Right direction, but too speculative to port as-is without knowing the blast radius.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Sep 3, 2026
@pavankumar-vh pavankumar-vh mentioned this pull request Sep 3, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pavankumar-vh @codebuff-team